Skip to content

perf(ci): run tests in-process multi-threaded (drop --test-threads=1) - #1225

Merged
yvgude merged 12 commits into
yvgude:mainfrom
andig:perf/test-threads-parallel
Jul 27, 2026
Merged

perf(ci): run tests in-process multi-threaded (drop --test-threads=1)#1225
yvgude merged 12 commits into
yvgude:mainfrom
andig:perf/test-threads-parallel

Conversation

@andig

@andig andig commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

CI ran the whole Rust suite with --test-threads=1 — an env-race legacy from v3.3.5 that serialized ~70 test binaries, including the heavy property/fuzz/benchmark suites, and dominated the job's wall clock. cargo-nextest was the first attempt (#1206) and was reverted: its process-per-test model re-pays this crate's heavy static-link cost, coming out 33-80% slower. This PR removes the reasons for the flag, then drops it.

Rebased onto main, so the Linux Test job's mold -run linker wiring comes from main rather than being duplicated here. Nothing else about the Test job changes — the diff is the flag plus the stale comment block above it.

What made the flag necessary, and what replaces it

  1. Env mutation. std::env::set_var is not thread-safe (unsafe in Rust 2024). Every env-mutating test serializes on data_dir::test_env_lock(), reworked to be reentrant (per-thread depth counter) so a test and its setup() / isolated_data_dir() helper can both take it without self-deadlocking. 71 unit tests that lacked it now hold it.

  2. Integration binaries. The rust/tests/*.rs binaries justified their unsafe set_var with "the project's test suite always runs with --test-threads=1" in ~35 SAFETY comments — a claim that expires with the flag. All 28 env-mutating files audited: most were already sound on their own terms (a file-local ENV_GUARD/HOME_LOCK mutex, serial_test, or a single #[test] in the binary). Five relied purely on the flag and now take test_env_lock() for the whole test body. Every stale SAFETY comment is retargeted to the guarantee that actually holds.

  3. Wall-clock assumption. hebbian_eviction_bonus_is_wired needed two store() calls inside a real 500ms co-access window. CoAccessMatrix::set_burst_window (test-only) makes it deterministic instead of hoping scheduling cooperates.

Two real defects the parallel run exposed

Running the suite under threads did not merely flake — it hung, reproducibly, at 6246/9089 tests with the process idle:

  • Lock-order deadlock in src/dashboard/tests.rs. The file held both a local ENV_LOCK and the global test_env_lock, in opposite orders: most tests took test_env_lock then ENV_LOCK; three took ENV_LOCK then isolated_data_dir() (which takes test_env_lock internally). Two threads on the opposite orders wedged the binary. ENV_LOCK is redundant with the global lock, so it is deleted — along with its LOCK_ORDERING.md row.

  • Cross-test contamination in the savings ledger. Three tests in core::ocla::builtin::response_optimizer call optimize_response, which appends a proxy_response_optimizer event to the savings ledger, with no isolated data dir. Under threads that write lands in whichever isolated dir is active, so savings_ledger::delegates_to_verified_ledger read a foreign event as its first ledger line. Each writer now takes its own isolated_data_dir(). The same tests also shared one session id + response ref, so whichever ran second got a cache hit and zero delivered tokens — each request is now keyed per test.

Testing

  • cargo test --lib multi-threaded: 9073 passed, 15 ignored, 0 failed, ~54s wall (before these fixes it did not finish at all).
  • Full cargo test --no-fail-fast (lib + all integration binaries), multi-threaded, run twice locally on macOS: green except the two notes below.
  • Repeated targeted runs: --test main ×6 green, ro_config_sandbox ×6 green, --lib ×3 green.

Known, not fixed here

  • suite::ro_config_sandbox::doctor_fix_splits_mixed_install_out_of_config_dir failed once in two full-suite runs, and never in 6 isolated runs of its own binary or 6 of the whole main suite. It spawns lean-ctx doctor --fix as a subprocess with explicit HOME/XDG env; the main suite binary contains zero set_var calls, so this is not an in-process env race and not something this PR introduces — cargo already ran the binaries the same way under the old flag. Worth its own issue as a load-sensitive subprocess flake.
  • critical_module_integration::pathjail_blocks_traversal fails on my machine only, because my personal ~/.config/lean-ctx/config.toml lists /Users/…/htdocs in extra_roots, which is exactly what the test's ../../etc/passwd resolves under. Green with a clean config dir; CI has none.

Not touched: release.yml, dep-update.yml and the Coverage job still pass --test-threads=1, and the BM25 perf gate keeps it deliberately (it times a quiet runner). Those can follow once this has soaked on main.

🤖 Generated with Claude Code

andig and others added 10 commits July 24, 2026 14:03
Extracted from the closed nextest PR (yvgude#1206) — the fix is needed for
in-process parallel `cargo test` too. core::cache::tests::
hebbian_eviction_bonus_is_wired required two store() calls inside a real
500ms burst window; under any parallel test execution scheduling jitter can
delay one past it. CoAccessMatrix::set_burst_window (test-only) lets the test
widen the window so the association is deterministic. Production keeps 500ms.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Groundwork for dropping `--test-threads=1` (in-process parallel cargo test,
the real CI speedup — nextest yvgude#1206 was 33-80% slower). Under true thread
parallelism `std::env::set_var` races any concurrent env access (it is unsafe
in Rust 2024), so every env-mutating test must serialize on the shared
`data_dir::test_env_lock()`. This adds it to the 71 that lacked it.

Additive and safe under the current serialized CI: a no-op there, it only
matters once threads are enabled. Tests already holding the lock via
`isolated_data_dir()` are skipped (re-taking the non-reentrant Mutex would
self-deadlock). Env-*reading* tests that still race are being found empirically
by multi-threaded runs and will follow; the CI flip lands only once those are
green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 71-lock retrofit deadlocked: tests that acquire the lock via a setup()
helper (or isolated_data_dir) and ALSO got a top-level lock added took the
non-reentrant Mutex twice on one thread.

Make test_env_lock reentrant — a per-thread depth counter, only the outermost
acquire holds the underlying Mutex, nested acquires bump the count. Same thread
re-enters freely; other threads still block, so cross-test serialization (the
whole point, since std::env::set_var is not thread-safe) is unchanged. This
makes the lock composable: a test + its helpers can each take it without
coordination.

Return type changes from MutexGuard to a small TestEnvGuard RAII type; the two
helpers that stored the guard's type explicitly (config_bridge::setup's tuple,
ocla_integration_suite's IsolatedDataDir) are updated. Helpers backed by their
own local mutex (home, tokenizer, kernel_config, ann_cache, …) are untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The nextest approach (yvgude#1206) is closed; this branch enables in-process parallel
tests by dropping --test-threads=1. Three code comments still cited nextest as
the flake cause — retarget them to 'once tests run in parallel' so they don't
point at an abandoned path. Also trim 'Widen (or narrow)' to 'Override' (tests
only ever widen). Comments only, no behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…olds it

The 71-lock retrofit added `let _env_lock = test_env_lock()` to three
config_bridge tests, but each also calls `setup()`, which takes the same lock
(returned as the second tuple guard). The retrofit script couldn't see the
helper-held lock. With the reentrant lock the double-take is harmless, but the
extra line is dead weight — remove it and note that setup() owns the lock.
lock_ordering_check::all_static_locks_documented flagged two undocumented
static locks: TEST_ENV_MUTEX (data_dir.rs:161, new in this branch) and
PROVIDER_STATS (envelope_bridge.rs:37, main drift). Add both to
LOCK_ORDERING.md so CI passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The lock_ordering_check matches a lock as documented by name only on doc
lines that literally contain "Mutex"/"RwLock"; otherwise it falls back to a
path match that does location.replace("src/", ""). On Windows the scanned
path uses backslashes (src\core\...), so the replace no-ops and never matches
the forward-slash doc entry. The E7 row had no literal "Mutex" (all-caps
TEST_ENV_MUTEX != "Mutex"), so it passed on Linux/macOS but failed on Windows.
Add the OnceLock<Mutex<()>> type so name-matching applies cross-platform.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The env-lock groundwork so far covered src/** unit tests. The integration
binaries in rust/tests/ still justified their unsafe std::env::set_var with
'the project's test suite always runs with --test-threads=1' — a claim that
stops being true the moment the flag is dropped, in ~35 SAFETY comments.

Audited all 28 env-mutating files. Most were already sound on their own
terms: a file-local ENV_GUARD/HOME_LOCK mutex, serial_test, or a binary with
a single #[test] (nothing else runs in that process). Five relied purely on
the flag and now take the shared reentrant data_dir::test_env_lock() for the
whole test body. Every stale SAFETY comment is retargeted to the guarantee
that actually holds.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two real defects that only appear once tests run multi-threaded, both found
by running the suite under threads:

1. Deadlock (not a flake): src/dashboard/tests.rs held BOTH a file-local
   `ENV_LOCK` and the global `test_env_lock`, in opposite orders — most tests
   took `test_env_lock` then `ENV_LOCK`, while three took `ENV_LOCK` then
   `isolated_data_dir()` (which takes `test_env_lock` internally). Two threads
   hitting the opposite orders wedged the whole binary: the run stalled at
   6246/9089 tests with the process idle. `ENV_LOCK` is now redundant with the
   global lock, so it is deleted (and its LOCK_ORDERING.md row with it).

2. Cross-test ledger contamination: three tests in
   `core::ocla::builtin::response_optimizer` call `optimize_response`, which
   appends a `proxy_response_optimizer` event to the savings ledger, without an
   isolated data dir. Under threads that write lands in whichever isolated dir
   is currently active, so `savings_ledger::delegates_to_verified_ledger` read a
   foreign event as the first ledger line. Each now takes its own
   `isolated_data_dir()`.

Local `cargo test --lib` multi-threaded: 9073 passed, 15 ignored, 0 failed,
54.7s wall (previously it did not finish at all).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
optimize_response caches per (session_id, response_ref). Every test used
s1/resp:abc, so whichever ran second got a cache hit and zero delivered
tokens; the fixed serial order hid it. Tag each request per test.

Also drops the now-unused count_tokens import from the cache test module
(the burst-window injection replaced the tiktoken warm-up), which -Dwarnings
would fail CI on.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@andig
andig force-pushed the perf/test-threads-parallel branch from c62a2b0 to a04fea1 Compare July 24, 2026 13:27
@andig andig changed the title perf(test): groundwork for in-process parallel tests (drop --test-threads=1) perf(ci): run tests in-process multi-threaded (drop --test-threads=1) Jul 24, 2026
@andig
andig marked this pull request as ready for review July 24, 2026 13:28
andig and others added 2 commits July 24, 2026 15:30
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both CI Test jobs failed on timing, not correctness: ubuntu measured 733ms
for a 500-chunk attention assembly against a 350ms bound, and Windows blew a
50ms health-latency bound. The bounds already carried 5-10x slack for runner
contention, but that assumed a serialized suite — with tests running
multi-threaded the runner is never quiet, and no fixed budget survives.

Follow the yvgude#581 pattern instead: the 15 wall-clock assertions in
performance_stress and context_kernel::perf_benchmark are now gated on
LEAN_CTX_PERF_GATE=1, and CI's existing single-threaded perf step runs all of
them. The workload and every correctness assertion still run in the normal
matrix, so the tests keep their non-timing value everywhere.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yvgude
yvgude merged commit ba25470 into yvgude:main Jul 27, 2026
25 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants